Use gossip in drone and wallet to identify leader ports #1002

CriesofCarrots · 2018-08-17T22:44:36Z

Fixes #934 and unblocks #920

CriesofCarrots · 2018-08-17T22:48:10Z

@pgarg66, this should work with your pr #920
Currently, this code has drone/wallet still reading leader.json, and then pulling the leader's ncp address from there.

garious · 2018-08-19T05:21:09Z

That's too much code to copy-paste. A utility function in thin_client.rs, perhaps?

Also, is this really what we want? On Google Cloud, will this return the public addresses a client would need or the private ones the network is using to minimize costs?

aeyakovenko · 2018-08-19T15:53:03Z

the public/private IPs are complicated. Maybe the network should advertise many IPs, in order of preference, but a single set of ports for all the services. The clients can find whatever works for them. But the above should probably be addressed in a different PR/issue.

aeyakovenko · 2018-08-19T15:55:07Z

src/bin/drone.rs

+    crdt.write().unwrap().insert(&leader_entry_point);
+
+    // Block until leader's correct contact info is received
+    while crdt.read().unwrap().leader_data().is_none() {}


this needs a sleep

Sure. Between the crdt write and read? How long and why?

this will spin the thread without yielding to the rest of the system to actually process anything to get the leader_data. if you sleep, that tells the OS that it can do other useful things. 100ms is probably good enough.

maybe pull this out into a function that polls for X seconds and returns an error if it fails. otherwise we may end up with this stuck in a misconfigured test.

The recent push implements a configurable timeout for both wallet and drone gossip-poll. (Timeout can also be none, if blocking is desired.) I plugged in some starting values in the bash scripts that are reasonable for my local machine. @aeyakovenko , do you have a feel for how long is reasonable for tests/automation? Maybe a question for @mvines ?
Presumably the wallet timeout can be short, since we expect a full node to be up, but I'm not clear on how long we would expect between drone and leader boot.

garious · 2018-08-20T21:28:58Z

src/thin_client.rs

+        }
+    }
+
+    exit.store(true, Ordering::Relaxed);


ncp.close(): https://github.com/solana-labs/solana/blob/master/src/ncp.rs#L63

garious · 2018-08-21T04:18:18Z

Hey @rob-solana, are you keeping an eye on the CLI options from all our various apps? This PR adds -w for "wait" where the default is to block. Seems a little funky to me. Thoughts?

rob-solana · 2018-08-22T06:12:35Z

If I read this correctly, the timeout is in the user's hands, i.e. if no -w argument is presented, the code keeps trying forever. That sounds correct to me.

rob-solana · 2018-08-22T06:18:25Z

src/bin/drone.rs

+            Arg::with_name("wait")
+                .short("w")
+                .long("wait")
+                .value_name("NUMBER")


clap is pretty awesome... I recommend you pass something that looks like help to value_name().

when you ask for help with this code you see something like

--wait NUMBER time to wait for leader info from gossip

What units? Why does this thing need "leader info"?

Were I the user, I'd prefer something that conveyed the units and better captured semantics, e.g.:

--timeout SECS wait at most SECS seconds to get necessary gossip from the network

rob-solana

some things ok, I think. some ergonomics polish might be worth the time

garious · 2018-08-22T15:01:30Z

I agree --timeout SECS would convey more meaning. And I'd be okay if there was no short form for this option (-w/-t), since it may only be used in automation. Using the long form would make those scripts more readable. @CriesofCarrots can you make that change?

CriesofCarrots · 2018-08-22T15:22:54Z

Great suggestions!

garious · 2018-08-22T21:54:53Z

@aeyakovenko (or @mvines) should this PR be blocked on that public/private IP issue?

garious · 2018-08-23T16:54:16Z

@CriesofCarrots, crickets. Merge it at your leisure.

- Add utility function - Add thread sleep - Enable configurable timeout for gossip poll

solana-grimes · 2018-08-23T18:51:07Z

💔 Unable to automerge due to CI failure

) * Bump eslint from 7.13.0 to 7.17.0 in /token-lending/js Bumps [eslint](https://github.com/eslint/eslint) from 7.13.0 to 7.17.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](eslint/eslint@v7.13.0...v7.17.0) Signed-off-by: dependabot[bot] <[email protected]> * fix: add mkdirp dep Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Justin Starry <[email protected]>

…olana-labs#1002) * replay: only vote on blocks with >= 32 data shreds in last fec set * pr feedback: pub(crate), inspect_err * pr feedback: error variants, collapse function, dedup * pr feedback: remove set_last_in_slot, rework test * pr feedback: add metric, perform check regardless of ff * pr feedback: mark block as dead rather than duplicate * pr feedback: self.meta, const_assert, no collect * pr feedback: cfg(test) assertion, remove expect and collect, error fmt * Keep the collect to preserve error * pr feedback: do not hold bank_forks lock for mark_dead_slot

CriesofCarrots requested a review from pgarg66 August 17, 2018 22:44

pgarg66 approved these changes Aug 17, 2018

View reviewed changes

CriesofCarrots requested a review from aeyakovenko August 18, 2018 00:28

aeyakovenko reviewed Aug 19, 2018

View reviewed changes

CriesofCarrots force-pushed the drone-wallet-gossip branch from 6fdf076 to 76aec9b Compare August 20, 2018 20:05

garious approved these changes Aug 20, 2018

View reviewed changes

CriesofCarrots force-pushed the drone-wallet-gossip branch 3 times, most recently from 503fee3 to 20a0abd Compare August 21, 2018 04:06

garious assigned rob-solana Aug 21, 2018

rob-solana reviewed Aug 22, 2018

View reviewed changes

rob-solana approved these changes Aug 22, 2018

View reviewed changes

CriesofCarrots force-pushed the drone-wallet-gossip branch from 20a0abd to 2a1374d Compare August 22, 2018 15:22

garious assigned aeyakovenko and unassigned rob-solana Aug 22, 2018

Tyera Eulberg added 3 commits August 23, 2018 12:30

Drone: use gossip to identify leader's port config

2f1228c

Wallet: use gossip to identify leader's port config

a85a0be

Improve gossip use for drone and wallet

7d195e4

- Add utility function - Add thread sleep - Enable configurable timeout for gossip poll

CriesofCarrots force-pushed the drone-wallet-gossip branch from 2a1374d to 7d195e4 Compare August 23, 2018 18:30

CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Aug 23, 2018

solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Aug 23, 2018

CriesofCarrots merged commit d4c4121 into solana-labs:master Aug 23, 2018

CriesofCarrots deleted the drone-wallet-gossip branch August 23, 2018 23:13

pgarg66 mentioned this pull request Aug 25, 2018

Fix crash in fullnode when poll_get_balance() returns error #1058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gossip in drone and wallet to identify leader ports #1002

Use gossip in drone and wallet to identify leader ports #1002

CriesofCarrots commented Aug 17, 2018 •

edited

Loading

CriesofCarrots commented Aug 17, 2018

garious commented Aug 19, 2018

aeyakovenko commented Aug 19, 2018 •

edited

Loading

aeyakovenko Aug 19, 2018

CriesofCarrots Aug 19, 2018

aeyakovenko Aug 19, 2018

CriesofCarrots Aug 20, 2018

garious Aug 20, 2018

garious commented Aug 21, 2018

rob-solana commented Aug 22, 2018

rob-solana Aug 22, 2018 •

edited

Loading

rob-solana left a comment

garious commented Aug 22, 2018

CriesofCarrots commented Aug 22, 2018

garious commented Aug 22, 2018

garious commented Aug 23, 2018

solana-grimes commented Aug 23, 2018

Use gossip in drone and wallet to identify leader ports #1002

Use gossip in drone and wallet to identify leader ports #1002

Conversation

CriesofCarrots commented Aug 17, 2018 • edited Loading

CriesofCarrots commented Aug 17, 2018

garious commented Aug 19, 2018

aeyakovenko commented Aug 19, 2018 • edited Loading

aeyakovenko Aug 19, 2018

Choose a reason for hiding this comment

CriesofCarrots Aug 19, 2018

Choose a reason for hiding this comment

aeyakovenko Aug 19, 2018

Choose a reason for hiding this comment

CriesofCarrots Aug 20, 2018

Choose a reason for hiding this comment

garious Aug 20, 2018

Choose a reason for hiding this comment

garious commented Aug 21, 2018

rob-solana commented Aug 22, 2018

rob-solana Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

rob-solana left a comment

Choose a reason for hiding this comment

garious commented Aug 22, 2018

CriesofCarrots commented Aug 22, 2018

garious commented Aug 22, 2018

garious commented Aug 23, 2018

solana-grimes commented Aug 23, 2018

CriesofCarrots commented Aug 17, 2018 •

edited

Loading

aeyakovenko commented Aug 19, 2018 •

edited

Loading

rob-solana Aug 22, 2018 •

edited

Loading